Avoiding extra byte copy inside FileSystemImpl.readFileInternal by doxlik · Pull Request #5677 · eclipse-vertx/vert.x

doxlik · 2025-08-13T16:24:05Z

Motivation:

In this short PR I did optimization that previously was discussed with @franz1981 in this issue: #5665 regarding avoiding of extra bytes copy in FileSystemImpl.readFileInternal method.

Additional notes:

I could not either "mvn package" or "mvn test" as there multiple Http2 related files inside master is not compiling now.
It is my totally first contribution, sorry if I missed something.

Conformance:

Everything is signed, my eclipse account is identical to github one.

vertx-core/src/main/java/io/vertx/core/file/impl/FileSystemImpl.java

franz1981 · 2025-08-14T03:25:33Z

vertx-core/src/main/java/io/vertx/core/file/impl/FileSystemImpl.java

          Path target = resolveFile(path).toPath();
          byte[] bytes = Files.readAllBytes(target);
-          return Buffer.buffer(bytes);
+          ByteBuf bb = Unpooled.wrappedBuffer(bytes);


@vietj : this is ok? Or you prefer to add a new constructor to `BufferImpl' which still uses the existing Vertx heap types eg VertxUnsafeHeapByteBuf/VertxHeapByteBuf ?
The reason I'm asking is:

Unpooled::wrappedBuffer still have Netty reference count checks under the hood, implemented - which have a cost

IIRC Vertx itself contains checks to make sure the allocator of a vertx buffer is "known" to vertx e.g.

vert.x/vertx-core/src/main/java/io/vertx/core/net/impl/VertxHandler.java

Lines 48 to 67 in ff78924

public static ByteBuf safeBuffer(ByteBuf byteBuf) {

Class<?> allocClass;

if (byteBuf != Unpooled.EMPTY_BUFFER &&

((allocClass = byteBuf.alloc().getClass()) == AdaptiveByteBufAllocator.class

|| allocClass == PooledByteBufAllocator.class

|| byteBuf instanceof CompositeByteBuf)) {

try {

if (byteBuf.isReadable()) {

ByteBuf buffer = VertxByteBufAllocator.DEFAULT.heapBuffer(byteBuf.readableBytes());

buffer.writeBytes(byteBuf, byteBuf.readerIndex(), byteBuf.readableBytes());

return buffer;

} else {

return Unpooled.EMPTY_BUFFER;

}

} finally {

byteBuf.release();

}

}

return byteBuf;

}

One pain point is that AFAIK Netty's heap buffers (one of the two variant) doesn't expose in public to directly set the wrapped buffer - and that's why Netty itself is using only non-Unsafe variants in Unpooled::wrappedBuffer(byte[]): if we want to do this right in Vertx we could leverage a single implementation (i.e.non-unsafe) as well or modify Netty to expose it for Unsafe variants (I can do it if needed)

doxlik · 2025-08-14T08:05:15Z

@franz1981 thank you for your comments, I removed unrelated changes. Actually they were made by Intelij IDEA code autoformatting and I thought that maybe they are okay, but I agree that they create unnecessary noise, so next time I will try to avoid any of them.

franz1981 · 2025-08-17T06:44:02Z

Fyi @vietj

tsegismont

Thanks @doxlik and @franz1981

How about changing the perform method body to:

      public Buffer perform() {
        try {
          Path target = resolveFile(path).toPath();
          try (FileChannel fc = FileChannel.open(target, StandardOpenOption.READ)) {
            BufferInternal res = BufferInternal.buffer(); // create with file size hint?
            long length = fc.size();
            res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast
            return res;
          }
        } catch (IOException e) {
          throw new FileSystemException(getFileAccessErrorMessage("read", path), e);
        }
      }

It's ok to use BufferInternal in Vert.x impl classes and Netty can take care of loading and writing the file bytes appropriately.

franz1981 · 2025-09-02T02:41:26Z

@tsegismont

It looks like

https://github.com/netty/netty/blob/f922892f44743411ce89e8259d705ed9c9d3763e/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java#L292

that's going to allocate the internal NIO buffer which usually is not used in the Vertx heap ones (the internal NIO buffer is something used by Netty while sending data over the wire while communicating w NIO JDK layers...).
But It's still a good suggestion and still an improvement vs what we got - while retaining the same behaviour as we have now

@doxlik wdyt?

doxlik · 2025-09-03T08:23:42Z

@franz1981 Regarding the suggestion, what if file really big and bigger than 2 gb? Then here " res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast" even if we check the size, we still need to fallback to something that can handle it, either wrapping or have some "while" loop until fully read the file to the buffer.

Regarding generally low level ByteBuffs stuff, it is super interesting but I have almost no solid knowledge of all its internals, so in terms of buffers / allocators optimization I fully trust to you.

tsegismont · 2025-09-08T13:04:35Z

@franz1981 Regarding the suggestion, what if file really big and bigger than 2 gb? Then here " res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast" even if we check the size, we still need to fallback to something that can handle it, either wrapping or have some "while" loop until fully read the file to the buffer.

I think it's safe to stick to the behavior of java.nio.file.Files#readAllBytes

If the size is bigger than java.nio.file.Files#MAX_BUFFER_SIZE, an OutOfMemoryError error is thrown

doxlik · 2025-09-19T17:29:11Z

@tsegismont then if we want stick to behavior of java.nio.file.Files#readAllBytes, then should we move to the channel approach as you suggested or we can simply leave the current code that already use readAllBytes?

doxlik · 2025-09-19T17:31:07Z

@franz1981 @vietj I also wanted to ask, what will be further process of the review / PR? I mean is it kinda in direct queue of pending PR's for Julien's review / approval or we are waiting for Vert.x 5.1.0 to proceed with this PR?

tsegismont · 2025-09-22T13:04:47Z

@tsegismont then if we want stick to behavior of java.nio.file.Files#readAllBytes, then should we move to the channel approach as you suggested or we can simply leave the current code that already use readAllBytes?

We should move to the channel approach but, for the error cases, we can reproduce the behavior of java.nio.file.Files#readAllBytes

Signed-off-by: doxlik <pirozhokkokosovich@gmail.com>

doxlik · 2025-09-23T09:03:58Z

@tsegismont I published solution where tried to go FileChannel option but within error logic of readAllBytes. Check when you have time, please.

Closes eclipse-vertx#5677 The improvement consists in creating the buffer upfront and using Netty API to read the file channel. Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

tsegismont · 2025-09-23T16:53:59Z

Thank you for the updates @doxlik

I've created #5730 , that supersedes this one.

Closes eclipse-vertx#5677 The improvement consists in creating the buffer upfront and using Netty API to read the file channel. Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

doxlik force-pushed the refactor/optimizing-blocking-file-read branch 3 times, most recently from 40030e7 to 0ee2869 Compare August 13, 2025 16:53

franz1981 reviewed Aug 14, 2025

View reviewed changes

tsegismont reviewed Aug 27, 2025

View reviewed changes

doxlik added 2 commits September 23, 2025 12:53

Avoiding extra byte copy inside readFileInternal

4ec8e77

Signed-off-by: doxlik <pirozhokkokosovich@gmail.com>

removed unrelated changes

fabebae

doxlik force-pushed the refactor/optimizing-blocking-file-read branch from 735560e to fabebae Compare September 23, 2025 08:53

modified method to use channel approach

3eab03c

tsegismont mentioned this pull request Sep 23, 2025

Avoid extra byte copy inside FileSystemImpl.readFileInternal #5730

Open

tsegismont closed this Sep 23, 2025

	public static ByteBuf safeBuffer(ByteBuf byteBuf) {
	Class<?> allocClass;
	if (byteBuf != Unpooled.EMPTY_BUFFER &&
	((allocClass = byteBuf.alloc().getClass()) == AdaptiveByteBufAllocator.class
	\|\| allocClass == PooledByteBufAllocator.class
	\|\| byteBuf instanceof CompositeByteBuf)) {
	try {
	if (byteBuf.isReadable()) {
	ByteBuf buffer = VertxByteBufAllocator.DEFAULT.heapBuffer(byteBuf.readableBytes());
	buffer.writeBytes(byteBuf, byteBuf.readerIndex(), byteBuf.readableBytes());
	return buffer;
	} else {
	return Unpooled.EMPTY_BUFFER;
	}
	} finally {
	byteBuf.release();
	}
	}
	return byteBuf;
	}

Conversation

doxlik commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

franz1981 Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

doxlik commented Aug 14, 2025

Uh oh!

franz1981 commented Aug 17, 2025

Uh oh!

tsegismont left a comment

Choose a reason for hiding this comment

Uh oh!

franz1981 commented Sep 2, 2025

Uh oh!

doxlik commented Sep 3, 2025

Uh oh!

tsegismont commented Sep 8, 2025

Uh oh!

doxlik commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doxlik commented Sep 19, 2025

Uh oh!

tsegismont commented Sep 22, 2025

Uh oh!

doxlik commented Sep 23, 2025

Uh oh!

tsegismont commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

doxlik commented Sep 19, 2025 •

edited

Loading